Assertion failure: IsIdle(oldState), at /builds/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:132 #0 Checker::StartWriteOp() /src/xpcom/ds/PLDHashTable.h:130:5 #1 PLDHashTable::RemoveEntry(PLDHashEntryHdr*) /src/xpcom/ds/PLDHashTable.cpp:648:15 #2 nsBaseHashtable<nsAttrHashKey, RefPtr<mozilla::dom::Attr>, mozilla::dom::Attr*>::LookupResult::Remove() /src/obj-firefox/dist/include/nsBaseHashtable.h:209:14 #3 nsDOMAttributeMap::DropAttribute(int, nsIAtom*) /src/dom/base/nsDOMAttributeMap.cpp:123:11 #4 mozilla::dom::Element::UnsetAttr(int, nsIAtom*, bool) /src/dom/base/Element.cpp:2904:27 #5 nsDOMAttributeMap::BlastSubtreeToPieces(nsINode*) /src/dom/base/nsDocument.cpp:7805:20 #6 nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) /src/dom/base/nsDocument.cpp:7988:5 #7 AdoptNodeIntoOwnerDoc(nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:1524:42 #8 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:2443:5 #9 mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:885:45 #10 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13 #11 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15 #12 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16 #13 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12 #14 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /src/js/src/jit/BaselineIC.cpp:2589:14 #15 0x23e639502b21 (<unknown module>)

This goes back further than mozregression can bisect (~1yr) :(

I think this is a bogus assert, perhaps due to bug 1187782. As the comment in BlastSubtreeToPieces says: // This non-standard style of iteration is presumably used because some // of the code in the loop body can trigger element removal, which // invalidates the iterator. The important part of the blast method is this: auto iter = map->mAttributeCache.ConstIter(); ... nsCOMPtr<nsIAttribute> attr = iter.UserData(); ... <some code that removes an element from the hash table> Creating an iterator marks the hash table as active. Then the code grabs the data in the iterator, but it is just doing this to grab an arbitrary element. Then we remove something from the hash table, which causes the assertion failure when it checks if the hash table is in use. However, it isn't really in use, because |iter| is dead at this point. The fix is to scope |iter| more tightly so that it is destroyed by the time we remove something from the hash table. It is unfortunate that we don't already have a test for this in the tree.

I'll mark this sec-other for now, but it can be unhidden once somebody double checks my logic.

Nathan, does my reasoning sound right to you?

I'll try to reproduce this, since there is another BlastSubtreeToPieces bug which I haven't managed to reproduce and these could be similar issues.

(In reply to Andrew McCreight [:mccr8] from comment #4) > Nathan, does my reasoning sound right to you? This logic sounds correct; the element->UnsetAttr call is guaranteed to remove the attribute from the hashtable we're iterating over?

(In reply to Nathan Froyd [:froydnj] from comment #6) > This logic sounds correct; the element->UnsetAttr call is guaranteed to > remove the attribute from the hashtable we're iterating over? Yes, I think so.

Comment on attachment 8912926 [details] Bug 1403377 - Destroy hash table iterator before modifying hash table. https://reviewboard.mozilla.org/r/184242/#review189720

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 5092af0f6bfa -d f215f88437f1: rebasing 423163:5092af0f6bfa "Bug 1403377 - Destroy hash table iterator before modifying hash table. r=smaug" (tip) merging dom/base/crashtests/crashtests.list merging dom/base/nsDocument.cpp warning: conflicts while merging dom/base/crashtests/crashtests.list! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e003f2efac27 Destroy hash table iterator before modifying hash table. r=smaug

Can this ride the 58 train?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14) > Can this ride the 58 train? Yeah, this should have no affect on release builds. The only reason to uplift it would be if there's a lot of fuzzing of 57 and this error gets hit a lot there.